-
Notifications
You must be signed in to change notification settings - Fork 46
feat!: start using vim.lsp.config #178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
390149e to
ffaab8a
Compare
|
@tris203 will this break something for your plugin? I am planning on waiting to merge this at least until nvim 0.11.1, so probably 1-2 months or something at least, but I think most of this refactor is finished |
Yes, or at least our recommended configs. Rather than using I need to do the same with |
Yeah I know. I might try it out to see if I like that better
Yes that sounds good! We can coordinate on when we are merging these then |
|
I have tried to make this as nice for the users as I can, since I am not hard deprecating the exe, args and config options, and also not the default install location of roslyn. I have tried to still resolve those as previously, but just also issue a warning about it being deprecated, and point then in the correct direction to fix this. It feels harsh to hard deprecate these things without any warning. |
lsp/roslyn.lua
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting this warning (twice) when opening files.
My config looks like this:
vim.lsp.config['roslyn'] = {
cmd = {
"dotnet",
vim.fs.joinpath(vim.fn.stdpath("data"), "roslyn", "Microsoft.CodeAnalysis.LanguageServer.dll"),
"--logLevel=Information",
"--extensionLogDirectory=" .. vim.fs.dirname(vim.lsp.get_log_path()),
"--stdio",
},
filetypes = { 'cs' },
}I'm still using the legacy path, and would like to keep doing so. Shouldn't this check that vim.lsp.config.roslyn.cmd is not defined instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I will fix
|
Would it be possible to integrate with neovim's lsp completion? Did a quick test but couldn't make it work. {
"seblyng/roslyn.nvim",
branch = "vim.lsp.config",
ft = "cs",
opts = {
filewatching = "off", -- I'm on macOS and seems there's some watch-related issues
},
},
...
vim.lsp.config['roslyn'] = {
cmd = {
"dotnet",
vim.fs.joinpath(vim.fn.stdpath("data"), "roslyn", "Microsoft.CodeAnalysis.LanguageServer.dll"),
"--logLevel=Information",
"--extensionLogDirectory=" .. vim.fs.dirname(vim.lsp.get_log_path()),
"--stdio",
},
filetypes = { 'cs' },
}
...
vim.api.nvim_create_autocmd({ 'LspAttach' }, {
callback = function(args)
local client = assert(vim.lsp.get_client_by_id(args.data.client_id))
if (client:supports_method('textDocument/completion')) then
vim.lsp.completion.enable(true, client.id, args.buf, { autotrigger = true })
end
end,
})
vim.lsp.enable('roslyn')Have to turn filewatching since I'm getting this message (nvim 0.11 through Neovide, macOS): Looking at |
|
A couple of things:
|
|
Can you test it now to see if you get any errors? I would hope that it should work with your above snippet if you just remove the enable call |
|
No warnings, works on a However I'm not getting good results on a more involved project (Unity project with many csprojs). The definitions within a file resolve fine (references to fields, or nested classes), but it's unable to find definitions outside of it. Not sure if filewatching has anything to do. |
|
Is that any different from main branch? Are you using razor? |
No, same results. I had it working with 0.10 + cmp. Wanted to check if 0.11 + lsp completion would work, but seems like it's not there yet.
No |
|
Aaah you mean builtin completion isn't working with not finding definitions and such? Yeah, I have also tried it a little bit out but to no luck. Hopefully the issue with filtering at least can be fixed, because without it it seems unusable |
|
Interesting... I am only on mobile now, but I see where it happens, but don't know why. It seems like Do you do anything special for this to happen? Could you try to explain when this happens? Like, do you just open a cs file and then this happens immediately? It seems like, when reading the code, that this should happen when opening files under a specific directory, because it seems to happen when searching for the root solution. |
Yes exactly. nvim: 0.11 (release) I can help debug since I can repro always on this one project. |
|
Could you try the latest changes, and produce the error, and then do |
|
🤦 noticed the issue the moment I saw your log. I have a malformed solution named |
|
Ah okay, will try to have this in mind and see if I can improve the error message |
|
Not sure where to ask this, I'll try here (is there a discord or other channels to chit chat usage / development?) How do I cycle through overloads using the lsp? Is roslyn.nvim helping provide that? I was using |
Not for this plugin no
This should be in neovim core now with |
|
|
|
No, that is up to the server |
55b4c02 to
982d0ce
Compare
This could interfere with initializing the server properly. Can guide the user with using a table if they absolutely want. Even though it will not work with `Roslyn target` since I need to override on_init
|
@seblyng maybe we could join forces and got a minimal config working in the official nvim-lspconfig repo, I created an draft PR with some changes I looked up from this repository: |
|
Yeah I would be open to that! I can add a review in nvim-lspconfig with some of my opinions |
|
Now that roslyn_ls is supported in nvim-lspconfig, can we have a list of what this plugins offer more? If i'm not mistaken, it:
|
|
Yes I can try to summarize and add it to the readme. To add to what you are listing: There is a bit more complex root dir detection with trying to find the project file of the file you are opening, and then trying to see if that is part of the solution it finds. It can also find solutions recursively in child directories It is also implementing a couple of more handlers, but some of those at least might be accepted upstream if someone tries to send a PR there |
This I have asked for to be added to nvim-treesitter previously and also now after nvim-lspconfig added support, but they didn't want to add it unfortunately |
|
I think I want to merge this within max a couple of weeks so that I don't have to maintain two branches with potential new features/bug fixes. Do you have any reservations @tris203 ? I think the current state is these breaking changes:
|
|
Works for me rzls.nvim is >0.11 only already |
|
Alright, since razor is already 0.11, I am just going to merge this now |
It looks like they discretely actually added it after they closed your issue, if I interpret it right? https://github.com/nvim-treesitter/nvim-treesitter/blob/main/plugin/filetypes.lua |
|
Yeah, I actually noticed that myself browsing through reddit this morning. I will remove the mention of the difference from the README, but I will keep the line that registers it a little longer, assuming that not necessarily everyone has migrated to the main branch of nvim-treesitter yet |

This PR is bumping the minimum required neovim version to 0.11.
It also comes with a couple of soft breaking changes (unfortunately), and removing of deprecating options.
Hard deprecations
exeoption will be removedargsoption will be removedSoft deprecations
configoptions is deprecated. Please usevim.lsp.configTODO:
I will be daily driving this until I eventually merge this